fix(task-graph): legacy unbranded {$ref} read-side rewrap + binaryBackpressure default no-op#559
Closed
sroussey wants to merge 3 commits into
Closed
Conversation
…ary ports only
Pre-brand cache writes landed without the literal kind discriminator, so
subsequent isCacheRef probes treat them as ordinary objects and skip
rehydration / threshold logic. Re-wrap them in place at every declared
binary streaming port — scoped to the same set hydrateRefsBelowThreshold
uses so non-binary fields legitimately carrying a {$ref: string} shape
(JSON-Schema refs, user metadata) are NEVER auto-promoted.
The new isLegacyUnbrandedCacheRefShape helper is the strict "needs
upgrade" predicate; it rejects already-branded refs and shapes whose
size/mime hints have the wrong type. The lookup path upgrades in place
on cache hit, and hydrateRefsBelowThreshold accepts both branded refs
and the pre-brand shape as defence in depth on the streaming
finish-event path that bypasses lookup.
RunPrivateCacheRepo.getOutput deliberately does NOT re-wrap: that would
require walking arbitrary nested fields and would re-introduce the
shape-only collision risk the brand exists to close.
…ecute paths Tasks may call await ctx.binaryBackpressure() unconditionally; the JSDoc already promised a no-op default when the runtime does not install a real backpressure source. Only StreamProcessor was installing the router-aware version, so non-streaming execute paths handed tasks an IExecuteContext where the field was undefined - calling it threw TypeError. Install a module-private shared no-op in TaskRunner.executeTask and WhileTaskRunner.executeTask. Sharing the constant avoids per-call allocation so the unconditional-call pattern actually costs nothing. executePreview's IExecutePreviewContext is Pick<IExecuteContext, 'own'> so it intentionally has no binaryBackpressure slot - the preview path needs no change.
…StreamProcessor Adds two scenarios over the StreamProcessor + BinaryStreamRouter park/drain handshake that the existing StreamingBackpressure suite did not exercise: 1. abort-while-parked through StreamProcessor.run - confirms an abort on ctx.abortController plus a drain on the router settles the run within ~200ms and emits either stream_end or stream_error. Catches future regressions where an abort orphans a parked producer push. 2. multi-waiter drain isolation - two concurrently parked pushes on sibling routers must wake independently (draining one does not wake the other), and multiple parked pushes on the SAME router are all released by a single drain. Exercises the prev/res linked-callback chain in BinaryStreamRouter._awaitDrain / push.
2 tasks
Collaborator
Author
|
Closing in favour of PR #565. The fixes in this PR (legacy unbranded Generated by Claude Code |
sroussey
added a commit
that referenced
this pull request
Jun 11, 2026
…ift guard (#565) * fix(ai): export ChunkRetrievalInputSchema + add nightly schema-vs-type drift guard Cherry-picks the applicable parts of PR #558 that target main: - Export `ChunkRetrievalInputSchema` (renamed from module-private `inputSchema`) so the drift test can reference it; add JSDoc documenting the intentional if/then/else tightening over `FromSchema`. - Add `packages/ai/src/task/__tests__/types.test-d.ts`: vitest typecheck-mode assertions that AiChat/ToolCalling prompt types match their `FromSchema` resolution and that ChunkRetrieval's discriminated union stays stricter. - Exclude `*.test-d.ts` from `packages/ai/tsconfig.json` so the per-PR typecheck:budget gate stays fast. - Add `.github/workflows/nightly-typecheck.yml`: nightly + workflow_dispatch job that runs the drift test via `--typecheck --typecheck.only`. PRs #557 and #559 are not cherry-picked: they fix the binary-streaming framework (CacheRef branding, BinaryStreamRouter backpressure) which lives on a feature branch not yet merged to main. https://claude.ai/code/session_013v3PWUAdtJBnWKLbzF8nfe * chore: update bun.lock https://claude.ai/code/session_013v3PWUAdtJBnWKLbzF8nfe --------- Co-authored-by: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Stacks two follow-ups on top of PR #557 so they merge together.
H-1 — Read-side rewrap of legacy unbranded
{$ref}cache rowsPre-brand cache writes landed as
{ $ref, size?, mime? }without the literalkinddiscriminator that #557 introduced. SubsequentisCacheRefprobes then treat those rows as ordinary objects and skip threshold-based rehydration. We upgrade them in place — but only at schema-declared binary streaming ports — by reusing the samegetStreamingPorts(...).filter(p => p.mode === "binary")sethydrateRefsBelowThresholduses. Non-binary fields that legitimately carry a{$ref: string}shape (JSON-Schema refs, user metadata) are NEVER auto-promoted, so the shape-only collision risk #557 closes stays closed.Why option (b) over a
__cvbump: a__cvbump throws away every pre-brand cached binary write. Read-side rewrap upgrades those rows transparently.The new
isLegacyUnbrandedCacheRefShapehelper is a strict "needs upgrade" predicate; it rejects already-branded refs and shapes whosesize/mimehints have the wrong type.CacheCoordinator.lookupupgrades in place on cache hit, andhydrateRefsBelowThresholdaccepts both branded refs and the pre-brand shape as defence in depth on the streaming finish-event path that bypasses lookup.RunPrivateCacheRepo.getOutputdeliberately does NOT re-wrap — that would have to walk arbitrary nested fields without the schema in scope.H-2 —
binaryBackpressuredefault no-opIExecuteContext.binaryBackpressure's JSDoc already promised a "no-op default when the runtime does not install a real backpressure source." OnlyStreamProcessorwas installing the router-aware version, so non-streamingexecute()paths handed tasks anIExecuteContextwhose field was undefined — calling it threwTypeError. Install a module-private shared no-op inTaskRunner.executeTaskandWhileTaskRunner.executeTaskso the unconditional-call pattern actually costs nothing per call.executePreview'sIExecutePreviewContextisPick<IExecuteContext, "own">and intentionally has nobinaryBackpressureslot — the preview path needs no change.Test coverage
CacheRef.legacyShape.test.ts— Case A (threshold 0, ref upgraded); Case B (rehydratable viagetOutputByRef); Case C — the load-bearing scope guard: a non-binary port whose cached value is{$ref: "#/$defs/X"}(a legitimate JSON-Schema ref) MUST NOT be touched, andgetOutputByRefMUST NEVER be invoked. If Case C breaks, the fix is unsafe to ship.TaskRunner.binaryBackpressure.test.ts—execute()canawait ctx.binaryBackpressure()without throwing; repeated calls stay cheap;runPreview()still works.StreamingBackpressure.abortDrain.test.ts— abort-while-parked throughStreamProcessor.runsettles within ~200ms emittingstream_endorstream_error; sibling routers' waiters wake independently; multiple parked pushes on the same router are all released by a single drain (exercises theprev/reslinked-callback chain inBinaryStreamRouter._awaitDrain/push).Test plan
bun scripts/test.ts graph vitest— 839 / 839 tests pass across 86 filesbunx tsc -b packages/task-graph— cleanThis PR stacks on #557 and inherits its base path to main.
https://claude.ai/code/session_01562Z29a2UQDNBVAcJGyUoY
Generated by Claude Code